Cpp target: allow building runtime with newer C++ standards #3071

felixn · 2021-02-08T17:05:45Z

Currently, the C++ runtime compilation is forced to used the C++11 standard, since CMAKE_CXX_STANDARD 11 is set in runtime/Cpp/CMakeLists.txt.
When linking an executable built with a newer C++ standard (for example, C++17) against the runtime build with C++11, the following linker error occurs:

FAILED: src/antlr/demo 
: && /usr/bin/clang++-11  -fuse-ld=lld src/antlr/CMakeFiles/demo.dir/src/demo.cpp.o src/antlr/CMakeFiles/demo.dir/antlr4cpp_generated_src/TLexer/TLexer.cpp.o src/antlr/CMakeFiles/demo.dir/antlr4cpp_generated_src/TParser/TParser.cpp.o -o src/antlr/demo  src/antlr/antlr4_runtime/src/antlr4_runtime/runtime/Cpp/dist/libantlr4-runtime.a && :
ld.lld: error: undefined symbol: antlr4::ANTLRInputStream::ANTLRInputStream(std::basic_string_view<char, std::char_traits<char> >)
>>> referenced by demo.cpp:20 (../src/antlr/src/demo.cpp:20)
>>>               src/antlr/CMakeFiles/demo.dir/src/demo.cpp.o:(main)

This occurs due to the following snippet in ANTLRInputStream.h:

#if __cplusplus >= 201703L
    ANTLRInputStream(std::string_view input = "");
#else
    ANTLRInputStream(const std::string &input = "");
#endif

-> the runtime - built with C++11 - contains a different constructor than an executable - built with C++17 - tries to use

This pull request allows overriding the C++ standard for the runtime build - default is still C++11.
Therefore, projects using the C++ can choose to build the runtime with a newer standard.

@mike-lischke - what do you think?

mike-lischke · 2021-02-09T08:33:49Z

runtime/Cpp/CMakeLists.txt

  set(CMAKE_CXX_STANDARD_REQUIRED ON)
+  set(CMAKE_CXX_EXTENSIONS OFF)


Why would you want to disable CXX extensions? Is there a rational behind this decision?

The runtime doesn't need/builds without GNU C++ Extensions.
(And that's a good thing, because e.g. Clang might not support (all) the GNU C++ extensions).
So I feel like it's good to make it explicit, but I'm ok with reverting the change if you want.

mike-lischke · 2021-02-09T08:36:48Z

runtime/Cpp/cmake/ExternalAntlr4Cpp.cmake

@@ -104,6 +104,7 @@ else()
      CMAKE_CACHE_ARGS
          -DCMAKE_BUILD_TYPE:STRING=${CMAKE_BUILD_TYPE}
          -DWITH_STATIC_CRT:BOOL=${ANTLR4_WITH_STATIC_CRT}
+          # -DCMAKE_CXX_STANDARD:STRING=17 # if desired, compile the runtime with a different C++ standard


This setting should not be done in the cmake file, but be set by the outer project which needs to build ANTLR4. As such it makes not so much sense to have this here, even if it is only a comment.

Additionally, it should be documented somewhere (readme).

As far as I can tell, ExternalProject_Add() runs in a separate CMake instance, and does not use the same args as the outer project. So I've had to pass the build arguments explicitly here.
(The alternative seems too complicated: https://stackoverflow.com/a/48555098)

Either way, you're right, I have to update the readme - will do.

Replacing the snippet above with -DCMAKE_CXX_STANDARD:STRING=${CMAKE_CXX_STANDARD} builds the runtime with the same C++ standard as the outer project. Currently, I've added it as a commented out line. Do you want me to un-comment it and make it the new default behaviour? I think the risk of breaking things is pretty small, because projects using the ExternalAntlr4Cpp build flow have a local copy of ExternalAntlr4Cpp.cmake in their source tree anyways.

mike-lischke · 2021-02-09T08:37:42Z

Btw. if this is your first patch for ANTLR4 please also add yourself to the contributors.txt file.

felixn · 2021-02-09T08:43:26Z

Btw. if this is your first patch for ANTLR4 please also add yourself to the contributors.txt file.

Already done:

antlr4/contributors.txt

Line 251 in a60c32d

2019/11/17, felixn, Felix Nieuwenhuizhen, felix@tdlrali.com

Thanks for the quick feedback!

AdrianBunk · 2021-02-09T22:23:52Z

I would say commit 254b144 is bogus.

The problem is that this commit creates two different ABI versions of the library, depending on whether the library was built with a compiler set to >= C++17 or < C++17.

Exposing both in the header is wrong, and gives library users a link error at the end of the build when they use the option different from how the library was built.

In practice that currently means that code building with >= C++17 fails to link with the library.

The proper solutions would be that you either:
1 .compile your library with >= C++ 17 and require all library users to use C++17 with an #error on older C++, or
2. revert (or comment out) commit 254b144

mike-lischke · 2021-02-10T08:26:03Z

But the fact that there are 2 different ABI versions doesn't make it bogus. As with many other libraries you have to use the same C++ dialect for the importing code, as was used for the library to build it. That is common practice (and has occasionally led to some frustration).

Usually, and that's the recommended use of the C++ runtime, you should add it in source form to your project and build it as part of that. The build time is short and you avoid any trouble because of different build settings.

AdrianBunk · 2021-02-10T17:11:05Z

But the fact that there are 2 different ABI versions doesn't make it bogus. As with many other libraries you have to use the same C++ dialect for the importing code, as was used for the library to build it.

The only effect of commit 254b144 so far has been always breaking using >= C++17 when building against your library.

Currently CMakeLists.txt sets the standard when building the library itself to exactly C++11.
Commit 254b144 changes the header to an ABI the library cannot cannot even be built for when a library user uses >= C++17.

And if you really want different ABIs depending on the C++ version your library was built with, please expose only this ABI in the header instead of making the ABI in the header depend on something unrelated to how the library has been built.

mike-lischke · 2021-02-14T10:02:53Z

@parrt Currently two build targets fail here and it seems as if this patch has nothing to do with that. One is a C++ build where no output is generated after the build and the other is for Javascript with some test failures which need to be fixed elsewhere, before we can continue with accepting patches.

@felixn Once you updated the documenation (and the builds succeed again), we can accept your patch.

@AdrianBunk Commit #254b144 adds another constructor, which is only available when compiling with C++ 17 and above. So, if you build both the library and your app with C++ 17 it will work. The fixed setting of C++ 11 is what's being addressed in this patch (so you can choose afterwards), but otherwise I think the string_view constructor is ok.

ericvergnaud · 2021-02-14T11:13:50Z

@mike-lischke FYI if you click on details you can re-run a build from where it failed
(I just triggered one for the failing for cpp-recursion)
the JS issue has been fixed in parallel

mike-lischke · 2021-02-14T13:14:37Z

@ericvergnaud Thanks for the hint, but it seems I have not the privilege to rerun a job. So, atm, the JS job is still marked as failed.

AdrianBunk · 2021-02-14T17:23:33Z

@AdrianBunk Commit #254b144 adds another constructor, which is only available when compiling with C++ 17 and above. So, if you build both the library and your app with C++ 17 it will work.

As of today compiling the library with >= C++ 17 not possible.

The fixed setting of C++ 11 is what's being addressed in this patch (so you can choose afterwards), but otherwise I think the string_view constructor is ok.

I am a Debian Developer, and I ended up here due to the problems commit 254b144 is causing when trying to build some software from Oracle using C++ 17 with the version of your library in the upcoming Debian 11.

Exposing ABIs you are not providing in the headers is an absolute nightmare for distributions shipping binary builds of your library.
Iif you want to change library ABI depending on the C++ version used when compiling the library, please also generate the header so that it provides exactly the one ABI that is also provided by the library.

felixn · 2021-02-15T19:04:06Z

@felixn Once you updated the documentation (and the builds succeed again), we can accept your patch.

@mike-lischke - Done!

Iif you want to change library ABI depending on the C++ version used when compiling the library, please also generate the header so that it provides exactly the one ABI that is also provided by the library.

@AdrianBunk - what would that look like? Currently the header files are not generated at all, but are static files. Either way, I think that's out of scope of this PR? This PR is purely about enabling building the runtime with a C++ standard != C++11

AdrianBunk · 2021-02-15T19:17:31Z

Currently the header files are not generated at all, but are static files.

Are Linux distributions supposed to manually patch the headers themselves?

Either way, I think that's out of scope of this PR? This PR is purely about enabling building the runtime with a C++ standard != C++11

The scope should be enabling building code with C++ >= 17 against the library.
I still don't see why people here are so eager to create and keep multiple incompatible ABIs.

felixn · 2021-02-15T19:30:48Z

Are Linux distributions supposed to manually patch the headers themselves?

That's a rhetorical question, of course, but I don't think it's that helpful. Look, I asked "what would that look like?" because I don't know what it is exactly that you need. I'm not the author of the commit that's causing the issue nor all that involved in the antlr project aside from a few bug fixes, I'm just trying to get my use case (build with C++17) to work again. Still, I'm trying to figure out how we can address your use case as well, but I get the feeling that the changes required (generated header files?) are out of scope of this PR. So I think a separate issue or PR would be better. If you disagree, then it would be helpful to make to make concrete suggestions.

AdrianBunk · 2021-02-15T21:16:39Z

Are Linux distributions supposed to manually patch the headers themselves?

That's a rhetorical question, of course, but I don't think it's that helpful.

This is not a rhetorical question, this is pretty much the only alternative option.

Look, I asked "what would that look like?" because I don't know what it is exactly that you need. I'm not the author of the commit that's causing the issue nor all that involved in the antlr project aside from a few bug fixes, I'm just trying to get my use case (build with C++17) to work again. Still, I'm trying to figure out how we can address your use case as well, but I get the feeling that the changes required (generated header files?) are out of scope of this PR. So I think a separate issue or PR would be better. If you disagree, then it would be helpful to make to make concrete suggestions.

A revert can be a better option than trying to fix something that never worked.

Your use case (build with C++17) is broken because the headers are currently exposing a different ABI with C++ >= 17 that is never provided.
Instead of trying to provide the ABI, the simple solution would be to fix the header.

Status quo is that it was never possible to compile the library with C++ >= 17.
In other words, it was never possible to compile it for the second ABI introduced by commit 254b144.

I would suggest to edit ANTLRInputStream.h to change the #if __cplusplus >= 201703L into #if 0.
And keep it that way until everything becomes C++ >= 17 only. At that point the header should also get an #if __cplusplus < 201703L that gives an #error.

mike-lischke · 2021-02-16T08:24:02Z

@AdrianBunk You might have a valid point here, which we should investigate. However, that's not the purpose of this PR here. Instead please open a new issue and describe in more detail what the issue is with commit #254b144 is.

In order to ensure the current code builds with C++17 I did a local build in Xcode, which went fine (for both C++11 and C++17, without GNU extensions).

mike-lischke · 2021-02-16T08:25:31Z

@parrt This PR is ready to merge, once the builds are green again.

mike-lischke reviewed Feb 9, 2021

View reviewed changes

Felix Nieuwenhuizen added 4 commits February 15, 2021 18:50

whitespace cleanup

66bf523

allow override of CMAKE_CXX_STANDARD

98a3f7d

add example showing how to compile Cpp runtime with a different standard

03ee69b

add hints about C++ standard to readmes

a08edf8

felixn force-pushed the cxx-17 branch from 994afc9 to a08edf8 Compare February 15, 2021 18:57

mike-lischke approved these changes Feb 16, 2021

View reviewed changes

parrt added comp:build target:cpp labels Feb 16, 2021

parrt merged commit fde0b28 into antlr:master Feb 16, 2021

felixn deleted the cxx-17 branch February 16, 2021 20:08

ericvergnaud added this to the 4.9.2 milestone Mar 7, 2021

This was referenced Mar 12, 2021

Bump antlr4-runtime.version from 4.9.1 to 4.9.2 nbbrd/java-service-util#67

Merged

Bump antlr4.version from 4.5.1 to 4.9.2 KorAP/Koral#88

Closed

Bump version.antlr from 4.9.1 to 4.9.2 Captain-P-Goldfish/SCIM-SDK#127

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cpp target: allow building runtime with newer C++ standards #3071

Cpp target: allow building runtime with newer C++ standards #3071

felixn commented Feb 8, 2021

mike-lischke Feb 9, 2021

felixn Feb 9, 2021

mike-lischke Feb 9, 2021

felixn Feb 9, 2021

felixn Feb 15, 2021

mike-lischke commented Feb 9, 2021

felixn commented Feb 9, 2021

AdrianBunk commented Feb 9, 2021

mike-lischke commented Feb 10, 2021

AdrianBunk commented Feb 10, 2021

mike-lischke commented Feb 14, 2021

ericvergnaud commented Feb 14, 2021

mike-lischke commented Feb 14, 2021

AdrianBunk commented Feb 14, 2021

felixn commented Feb 15, 2021 •

edited

Loading

AdrianBunk commented Feb 15, 2021

felixn commented Feb 15, 2021

AdrianBunk commented Feb 15, 2021

mike-lischke commented Feb 16, 2021 •

edited

Loading

mike-lischke commented Feb 16, 2021

		set(CMAKE_CXX_STANDARD_REQUIRED ON)
		set(CMAKE_CXX_EXTENSIONS OFF)

Cpp target: allow building runtime with newer C++ standards #3071

Cpp target: allow building runtime with newer C++ standards #3071

Conversation

felixn commented Feb 8, 2021

mike-lischke Feb 9, 2021

Choose a reason for hiding this comment

felixn Feb 9, 2021

Choose a reason for hiding this comment

mike-lischke Feb 9, 2021

Choose a reason for hiding this comment

felixn Feb 9, 2021

Choose a reason for hiding this comment

felixn Feb 15, 2021

Choose a reason for hiding this comment

mike-lischke commented Feb 9, 2021

felixn commented Feb 9, 2021

AdrianBunk commented Feb 9, 2021

mike-lischke commented Feb 10, 2021

AdrianBunk commented Feb 10, 2021

mike-lischke commented Feb 14, 2021

ericvergnaud commented Feb 14, 2021

mike-lischke commented Feb 14, 2021

AdrianBunk commented Feb 14, 2021

felixn commented Feb 15, 2021 • edited Loading

AdrianBunk commented Feb 15, 2021

felixn commented Feb 15, 2021

AdrianBunk commented Feb 15, 2021

mike-lischke commented Feb 16, 2021 • edited Loading

mike-lischke commented Feb 16, 2021

felixn commented Feb 15, 2021 •

edited

Loading

mike-lischke commented Feb 16, 2021 •

edited

Loading